Begin deprecation cycle of the discrete pulse library #10222
Conversation
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 5444281151
💛 - Coveralls |
There was a problem hiding this comment.
Quick review for the first round. Overall this looks good to me. Do you have plans for continuous and sampler module deprecation? They are no longer useful after discrete is removed (Qiskit Dynamics user may use continuous to evaluate pulse value for upsampling).
| since="0.25.0", | ||
| additional_msg="The discrete pulses library, including zero() is pending deprecation." | ||
| " Instead, use the SymbolicPulse library to create the waveform with" | ||
| " pulse.Constant(amp=0,...).get_waveform().", |
There was a problem hiding this comment.
We can also define Zero function in the symbolic pulse module that wraps Constant with amp=0.
There was a problem hiding this comment.
I don't have a strong opinion here. I don't know how much people actually use zero amplitude constants as spacers (@wshanks correctly pointed out elsewhere that there are use cases where you would want something like this) but I suspect it doesn't warrant the extra clutter in the code base.
There was a problem hiding this comment.
It's just a problem of backward compatibility of import. Once we remove this module, people cannot find zero in the pulse namespace and there is no alternative approach in terms of import. Likely this pulse is not significantly important and can go in this way.
qiskit/pulse/library/discrete.py
Outdated
| " pulse.Square(...).get_waveform()." | ||
| " Note that pulse.Square() does not support complex values for `amp`." | ||
| " Instead, use two float values for (`amp`, `angle`)." | ||
| " Also note that the underlying `sign` function used to" |
There was a problem hiding this comment.
I cannot understand this message without reading code. Maybe we can say new symbolic implementation may return different value at t=0?
There was a problem hiding this comment.
Actually, the problem is not with t=0 because the first time slot is evaluated at t=0.5. The problem arises only when the sin function is evaluated to exactly zero. This might not even happen (as the tests I added demonstrate). This is somewhat of an edge case. I am not sure how to handle it correctly.
There was a problem hiding this comment.
Then you can write more details (possibly with example code) in release note and say "read release note for more details" here? I don't really like lengthy warning as it might be raised many times.
There was a problem hiding this comment.
I opted for matching the functional form of square() and Square(), so this discussion was almost mute. However, when I did that I noticed that the phase in square() was defined somewhat strangely, so I changed that, and now the warning addresses that - but it points the user to the documentation like you suggested.
releasenotes/notes/discrete-pulse-library-deprecation-3a95eba7e29d8d49.yaml
Outdated
Show resolved
Hide resolved
|
Thanks for quick review @nkanazawa1989 ! |
|
Thanks I just adde some suggestions in reply. We can probably deprecate continuous and sampler after we drop the discrete module? |
Summary
The discrete pulse library serves little purpose today, as the
SymbolicPulseclass and library offer the same functionality at reduced computational cost. This PR completes theSymbolicPulsecounterparts to the discrete library, and begins the deprecation cycle for it.Details and comments
The discrete pulse library serves little purpose today, as the
SymbolicPulseclass and library offer the same functionality at reduced computational cost. Furthermore, it is desired to change the naming convention of the pulses in the symbolic pulse library from camel case to snake case, to reflect the fact that they are no longer subclasses ofSymbolicPulse. This change is not possible because of naming conflicts with the discrete pulses. For these reasons, it was decided to deprecate the discrete library.This PR:
SymbolicPulses to the library, to recreate the functionality of all discrete pulses (namely,Sech,SechDeriv,GaussianDeriv,Square). Because of the aforementioned naming conflict, the functions use camel case notation.PendingDeprecationWarningfor all pulses in the discrete library, with detailed info about theSymbolicPulsecounterpart.With a few minor exceptions, the
SymbolicPulselibrary recreates the same behavior as the discrete library. The one notable difference is the adoption of the (amp,angle) representation in the symbolic library.Debatable decisions
SymbolicPulseform. WhileWaveforms can be added, the same is not true forSymbolicPulses - and this probably removes the prominent use case forGaussianDerivandSechDeriv. (One can consider a DRAG version with Sech instead of Gaussian, butSechDerivon its own makes little sense to me.)limit_amplitudeandlimit_amplitude_per_instance).